Skip to content

[native] Add TestAggregations to native-tests#24809

Merged
pramodsatya merged 4 commits intoprestodb:masterfrom
pramodsatya:agg_cpp_tests
Jun 13, 2025
Merged

[native] Add TestAggregations to native-tests#24809
pramodsatya merged 4 commits intoprestodb:masterfrom
pramodsatya:agg_cpp_tests

Conversation

@pramodsatya
Copy link
Contributor

@pramodsatya pramodsatya commented Mar 26, 2025 •
edited
Loading

Description

  1. Removes overriden testcase testCountDistinct in TestOptimizeMixedDistinctAggregations; the queries from this testcase are covered in base class AbstractTestAggregations.
  2. Parameterizes testcases in AbstractTestAggregations to support DWRF format.
  3. QDigest, TDigest types are now supported in Velox. Adds these types to supported parametric types in NativeTypeManager so the statistical digest aggregation tests do not return unsupported type error.
  4. Adds TestAggregations, TestOptimizeMixedDistinctAggregations to presto-native-tests.
  5. Fixes count(*) returning NULL for empty inputs when optimizer config optimize_mixed_distinct_aggregation is enabled on coordinator and native function namespace manager is enabled with sidecar.

Motivation and Context

Adds aggregation tests from presto-tests to run with native query runner in presto-native-tests.

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.
== RELEASE NOTES ==

General Changes
* Adds aggregation tests from ``presto-tests`` to run with native query runner in ``presto-native-tests``.

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Mar 26, 2025
@pramodsatya pramodsatya force-pushed the agg_cpp_tests branch 3 times, most recently from 9bfa7f6 to a492873 Compare March 27, 2025 04:31
@pramodsatya pramodsatya marked this pull request as ready for review March 27, 2025 04:51
@pramodsatya pramodsatya requested a review from presto-oss March 27, 2025 04:51
@prestodb-ci prestodb-ci requested a review from a team March 27, 2025 04:51
steveburnett
steveburnett previously approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (docs)

View .md file in GitHub to see how it will look when published. No issues. Thanks!

@pramodsatya pramodsatya mentioned this pull request Mar 27, 2025
Open
@pramodsatya
Copy link
Contributor Author

pramodsatya commented Mar 27, 2025

@jaystarshot, @aditi-pandit, @czentgr, @majetideepak, could you please help review this PR?

@aditi-pandit
Copy link
Contributor

aditi-pandit commented Mar 27, 2025

@pramodsatya : Thanks for these code changes.

Can you separate the storage format changes to a separate PR, and only keep the Aggregation test changes in this PR ? I see you have separate commits, but it will be simpler for reviewing one set without the other.

@pramodsatya
Copy link
Contributor Author

pramodsatya commented Mar 28, 2025

Thanks for the feedback @aditi-pandit, moved the storage format changes to a separate PR: #24820. Could you please take another look?

aditi-pandit
aditi-pandit reviewed Apr 14, 2025
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pramodsatya. Have a quick round of comments.

pramodsatya
pramodsatya commented Apr 22, 2025
View reviewed changes
Copy link
Contributor Author

@pramodsatya pramodsatya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aditi-pandit, could you please take another look?

aditi-pandit
aditi-pandit previously approved these changes Apr 25, 2025
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pramodsatya

@pramodsatya pramodsatya force-pushed the agg_cpp_tests branch 2 times, most recently from 6e979b2 to e2909ac Compare June 12, 2025 00:27
@pramodsatya pramodsatya requested a review from aditi-pandit June 12, 2025 00:29
@aditi-pandit
Copy link
Contributor

aditi-pandit commented Jun 12, 2025

@pramodsatya : Lets combine the last 2 commits. Else this PR is looking good to go. Thanks for iterating on it.

pdabre12
pdabre12 approved these changes Jun 12, 2025
View reviewed changes
Copy link
Contributor

@pdabre12 pdabre12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pramodsatya , LGTM.

@steveburnett
Copy link
Contributor

steveburnett commented Jun 12, 2025

Thanks for the release note entry! Minor formatting suggestion:

== RELEASE NOTES ==

General Changes
* Adds aggregation tests from ``presto-tests`` to run with native query runner in ``presto-native-tests``.

@pramodsatya
Copy link
Contributor Author

pramodsatya commented Jun 12, 2025

Thanks @aditi-pandit, @pdabre12, @steveburnett. Squashed the commits and updated release note, could you PTAL?

@aditi-pandit
Copy link
Contributor

aditi-pandit commented Jun 12, 2025

@tdcmeehan, @wanglinsong

hive-tests workflow are failing with error "Presto KeyStore certificate is expired: NotAfter: Thu Jun 12 01:05:49 CST 2025".
What's the follow up from our side ?

Error:  Failures: 
Error:    TestHiveSslWithKeyStore>AbstractTestQueryFramework.init:93->AbstractHiveSslTest.createQueryRunner:51 » Presto KeyStore certificate is expired: NotAfter: Thu Jun 12 01:05:49 CST 2025
Error:    TestHiveSslWithTrustStoreKeyStore>AbstractTestQueryFramework.init:93->AbstractHiveSslTest.createQueryRunner:51 » Presto KeyStore certificate is expired: NotAfter: Thu Jun 12 01:05:49 CST 2025

aditi-pandit
aditi-pandit approved these changes Jun 12, 2025
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pramodsatya

@tdcmeehan
Copy link
Contributor

tdcmeehan commented Jun 12, 2025 •
edited
Loading

Let's disable these tests on a separate commit since they break trunk. #25297 tracks the followup.

@tdcmeehan
Copy link
Contributor

tdcmeehan commented Jun 12, 2025

I raised #25302, feel free to cherry pick it.

@pramodsatya
Copy link
Contributor Author

pramodsatya commented Jun 12, 2025

I raised #25302, feel free to cherry pick it.

Thanks, could we land this first? I can rebase afterwards.

@pramodsatya pramodsatya merged commit 3dea808 into prestodb:master Jun 13, 2025
108 checks passed
@pramodsatya pramodsatya deleted the agg_cpp_tests branch June 13, 2025 16:08
@unidevel unidevel mentioned this pull request Jun 19, 2025
Closed
6 tasks
This was referenced Jul 4, 2025
Merged
Closed
Closed
This was referenced Jul 24, 2025
Merged
Merged
@prestodb-ci prestodb-ci mentioned this pull request Jul 28, 2025
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZacBlanco ZacBlanco ZacBlanco approved these changes

@tdcmeehan tdcmeehan tdcmeehan approved these changes

@pdabre12 pdabre12 pdabre12 approved these changes

@aditi-pandit aditi-pandit aditi-pandit approved these changes

@steveburnett steveburnett steveburnett left review comments

@majetideepak majetideepak Awaiting requested review from majetideepak

@czentgr czentgr Awaiting requested review from czentgr

@jaystarshot jaystarshot Awaiting requested review from jaystarshot jaystarshot is a code owner

@feilong-liu feilong-liu Awaiting requested review from feilong-liu feilong-liu is a code owner

@elharo elharo Awaiting requested review from elharo elharo is a code owner

@vivek-bharathan vivek-bharathan Awaiting requested review from vivek-bharathan

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Assignees

No one assigned

Labels

from:IBM PR from IBM

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pramodsatya @aditi-pandit @steveburnett @tdcmeehan @ZacBlanco @pdabre12 @prestodb-ci